feat(#494): direct Dashboard/Panel row actions, replacing the ⋯ overflow menus (phase 4) - #505
Merged
Merged
Conversation
…495 review) Post-merge review of PR #495 raised four defects; all four are fixed here, and three of them are foundations #494 (phase 4) builds three more row buttons and two more dialogs on. 1. Enter on a nested action button ran the ROW's command. The tree's keydown handler is on the list and its Enter arm calls preventDefault() + runs the focused row's single command, so Enter on the pencil opened the Dashboard — and the preventDefault() could swallow the button's own activation on the way out. The `⋯` and the orphan-variable trash shared the bug. Fixed in two independent layers: `isolateActivationKeys` stops Enter/Space propagating from each control (without preventing the default, so native activation still fires exactly once), and `handleTreeKeydown` ignores an Enter that originated on a button. Arrow/Home/End still reach the tree from a nested control, which is what keeps the row's composite tab stop navigable. 2. Rename failures closed the dialog and discarded the outcome. `commit()` now awaits `commitDashboardRename` and keeps the card open with the typed values on every unsuccessful outcome, showing one targeted diagnostic inline (`role="alert"`): a distinct sentence for a Dashboard that no longer resolves vs. the aggregate's own rejection diagnostic. Both actions are disabled while a write is in flight, so the same mutation cannot be submitted twice, and a late answer for a force-closed dialog is dropped rather than written into a detached card. 3. Dashboard creation had two commands with divergent failure behaviour. `application/dashboard-create.ts` is now the single one: it mints, appends against dequeue-time truth (falling back to a caller-supplied baseline for a workspace with no persisted aggregate), and `dashboardCreateMessage` normalizes the report. Both entry points call it; each keeps its own reveal policy, which is genuinely different. The placeholder previously said NOTHING on a rejected commit. 4. The modal had no dialog semantics. `openDialogShell` gives the card `role="dialog"`, `aria-modal="true"` and `aria-labelledby` pointing at a per-dialog title id. The two-field metadata dialog moves from `ui/dashboard-tree.ts` to `ui/dialog-shell.ts` as `openMetadataDialog` — #494's panel pencil is the second consumer hard rule 5 asks for before extracting. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
…low menus The Dashboard tree's rows now expose their operations as real buttons instead of hiding them behind a menu, following the Library Query row's pencil/trash vocabulary. No production row renders a `⋯` any more, so `DashboardTreeMenuItem`, `buildMenuButton` and the menu CSS are gone rather than left as an unused abstraction. Model (`application/dashboard-tree-model.ts`): `menu`, `deletable` and `renamable` — three expressions of the same idea — collapse into one `actions: DashboardTreeAction[]`, each carrying its accessible name, tooltip, fully-resolved target (stable ids only), an `unavailable` reason and, for the destructive ones, the confirmation sentence. Availability is the #427 exactly-one-owner rule, answered ONCE here through `buildQueryOwnershipIndex` rather than re-derived in the view; `OwnershipWorkspace.queries` narrows to `{ id }` so the tree's own loosened projection types can be indexed by it. Pure transforms (`dashboard/application/dashboard-removal.ts`): `removeDashboardPanel` removes a tile and exactly the dedicated query it owns — proving ownership first, composing `removeTileMembership` for layout normalization and grid-fallback regeneration, bumping the target revision exactly once — and `removeDashboardDocument` removes a document plus the queries its own tiles own, keeping any query another Dashboard also references. An orphaned `variableConfigs` entry deliberately survives a panel delete (#494 non-goal). Commits (`application/dashboard-delete.ts`, `dashboard-panel-metadata.ts`): both re-resolve their target inside the queued read-latest transform, so a confirmation or dialog that went stale while it was open commits nothing. The panel pencil edits the tile's OWNED QUERY through `renameSaved` — inheriting its linked-tab reconciliation — with a new dequeue-time `guard` on `patchSavedSpec` proving the tile still owns it. View: one `buildActionButton` per model action; unavailable controls render `aria-disabled` and inert with the reason as their tooltip, rather than vanishing; destructive ones confirm through the same anchored-menu primitive #447 already used; deleting a panel moves keyboard focus to the next sibling, else the previous, else the Panels group. The panel dialog warns when an imported tile title outranks the query name being edited. Not included: the Open-in-Dashboard focus button. It dispatches `focus: { kind: 'tile' }`, which is a proven no-op on flow-layout KPI tiles until #438 lands, and #494 forbids shipping a known no-op focus action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
…ELOG entry Real-browser coverage for what happy-dom cannot see: Enter and Space on a hover-revealed pencil (the #495 review defect, reached by tabbing rather than by `.click()`), the dialog's `role="dialog"` accessible name, the panel pencil's committed rename, the panel trash's confirmation and cascade, and — sabotage-checked against a label pinned to `flex: 0 0 auto` — that a long title still ellipsizes with both controls revealed instead of pushing them out of the pane. The action buttons deliberately keep the user agent's own focus ring: the chevron owns the `2px solid` outline channel, and #472 requires the row's targets to stay visibly distinguishable — the existing Tab-walk test catches a collision, which an earlier custom ring here caused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
…rmations Two independent review passes over the branch found one regression and one contract that no test could fail on. Both are fixed here. **Focus was lost to `<body>` after a successful metadata commit.** #495's review-2 fix made the dialog close only once the write ANSWERS — but that write repaints the tree first, so the trigger button the dialog captured at open time is detached by the time focus is handed back, and `focus()` on a detached node is a silent no-op. `renderDashboardTree`'s own restore deliberately declines to help, because focus was inside the body-mounted dialog rather than in the list. `returnFocusTo` now also accepts a resolver called at CLOSE time: the trigger while it is still on screen (Cancel/Escape, where nothing repainted), else the row, re-resolved by key. Sabotage-checked. **Deleting a Dashboard stranded focus the same way** — only the panel delete placed it. Both now do, and when the last Dashboard goes there is no row left to stand on, so focus lands on the tree's own search box. **A destructive confirmation opened with the destructive item focused.** `openMenu` autofocuses its first row, which for a confirmation means an Enter pressed out of momentum deletes a Dashboard. It takes an `initialFocus` hint now, and confirmations pass `'last'` — Cancel. **An UNAVAILABLE delete stopped looking like one**: `destructive` was derived from `act.confirm`, which the model nulls for an action it will never ask about, so a malformed-ownership trash lost its destructive styling and announced `aria-haspopup="dialog"`. It comes from the action's KIND now. **Cancel stays operable while a write is in flight.** Only the confirm is barred (that is what prevents a double submit); Escape and the backdrop were never gated, so a visibly dead Cancel just read as a wedged dialog. Tests that could not fail, now able to: the same-row `cancelFor` (all three existing tests only exercised the other-row half, so deleting the call left the suite green), the pencil/trash glyph mapping (swapping the icons passed everything), two `not.toBe('')` assertions that also pass on a missing attribute, and the e2e post-delete focus check, which asserted row text rather than `document.activeElement`. A keyboard-driven delete now proves the whole path in a real browser, including that the landing row is `:focus-visible`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
This was referenced Jul 27, 2026
Open
…<body>
A third review pass raised five findings. I verified each against the code
before acting: three reproduce exactly as described, one is real but was
overstated, and one turns on data the persistence layer forbids. All five are
addressed; each fix is sabotage-checked (reverting all of them together fails
twelve tests, spread across every area).
**The delete discarded the identity the confirmation named.** The row captured
`{dashboardId, tileId, queryId}` but `PanelDeleteTarget` dropped `queryId`, so
the transform deleted whatever the tile pointed at AT DEQUEUE TIME. Verified:
with the tile re-pointed from `qa` to `qb` between confirmation and commit, the
transform deleted `qb` — a state that is valid before and after, so nothing else
would have noticed. `queryId` is threaded through now and the tile must still
reference it (`tile-retargeted`).
The same transform resolved a tile with `.find` and removed with `.filter`, so a
duplicated tile id would have removed BOTH tiles (verified: 2 tiles in, 0 out,
one query orphaned), and a duplicated query id would have deleted every document
carrying it. Both now refuse (`tile-duplicate`) rather than resolving an
ambiguous id by picking one; the Dashboard cascade likewise keeps a query id
carried by two documents. Note these two states are NOT reachable from storage —
`dashboard-duplicate-tile-id` and `workspace-duplicate-query-id` are
error-severity diagnostics, and both decode-on-load and encode-on-commit reject
any workspace carrying one — so this is defence in depth, not a live data-loss
path. It is three lines, and #494 asks for fail-closed.
**A wrong-role query was editable and deletable.** A tile may only reference a
panel-role query; the validator says so (`dashboard-setup-reference`). Nothing
in the tree, the metadata guard or the delete checked it, so a tile referencing
a Setup query offered live controls whose use would "repair" the workspace by
destroying the evidence. `queryDashboardRole` is now consulted in all three, and
the controls render `aria-disabled` with their own reason. Same reachability
caveat as above.
**Deleting the only search match stranded focus on `<body>`.** Verified: rows
empty, `activeElement` BODY, `keyboardRowKey` null. The successor was chosen
before the write and applied blindly afterwards, but a search matching only the
deleted panel takes its group and its Dashboard off screen too. The pre-commit
choice is a preference now, re-checked against the rows that actually exist
after the repaint, with the search box as the floor. An empty render also
records its (empty) row list instead of leaving the previous one behind.
**Closing a stale dialog could lose focus the same way** — verified BODY when
the row was deleted while the dialog was open. The resolver falls back to the
tree's current roving row, then the search box.
**Repeated activation opened duplicate modals.** Verified: three clicks, three
backdrops, three `#dash-rename-name` elements. `openDialogShell` now enforces
the single-modal invariant its own header always claimed; the stale-slot guard
stays reachable through a second `close()` on an already-replaced handle.
The tree fixture's `mutateWorkspace` now projects AND repaints what it commits,
the way a real commit does — without that, post-delete assertions were made
against rows the write was supposed to have removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
Collaborator
Author
Review round 3 — verification notesFive findings raised. Each was checked against the code before acting; the fixes are in Confirmed and reproduced:
One correction to the framing. The malformed inputs behind 1b, 1c and 2 cannot reach the transform from storage. All five are fixed — see the commit message for the mechanics. Also worth recording: the tree's unit fixture never projected or repainted what it committed, so several post-delete assertions were being made against rows the write should have removed. It does both now, which is what made findings 3 and 5 reproducible at the unit level at all. Gate after the fixes: |
Review round 4. All four findings verified in the code first; each fix is sabotage-checked (reverting them together fails five tests). **`DialogHandle.close()` was not idempotent.** Now that opening a dialog force-closes the previous one, a caller can legitimately still hold a handle whose dialog is gone — and a second `close()` re-ran the focus restore (stealing focus from behind the modal that replaced it) and the caller's `onClose`. One `torndown` flag per handle. The previous round's double-close test was fair game for the criticism: it passed no `returnFocusTo` and no `onClose`, so it only proved the replacement stayed mounted. The new one asserts both. **The model offered controls the commands would refuse.** The tree's projection collapses duplicate query ids into one map entry (last wins), so availability was decided from whichever document won — while both commit paths require exactly one. A pencil that opens a dialog only to refuse at the end of it is worse than one that says up front why it cannot act. Cardinality is counted during derivation now, with its own reason. **A description-only tile override warned about nothing.** The viewer resolves a tile's body text as `tile.description || query description`, exactly as it resolves the heading from `tile.title || query name`. The note detected only the title, so an imported tile carrying just a description let the user edit a query description the tile goes on ignoring, silently. Both are detected independently and the sentence is composed from what is actually masked. **The whole-Dashboard delete cascaded into a non-panel query** while the single-panel delete refuses one — answering the reviewer's open question in the direction the narrower command already set: a tile referencing a Setup-role query is malformed data, and destroying that query as a side effect of removing its Dashboard is the cascade #494's fail-closed rule forbids. It survives as a Library query. CHANGELOG: the phase-3 entry still described the pencil as sitting beside the `⋯` and said there was no Dashboard trash — both superseded within the same unreleased section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
PR #499 (#466/#501) landed on the same files. Four conflicts, resolved as: **`src/ui/menu.ts` — take main's API, drop mine.** Both branches fixed the same hazard (a confirmation autofocusing its destructive row, so an Enter pressed out of momentum destroys something). Main added a per-row `autofocus: true`; this branch had added a menu-level `initialFocus: 'first' | 'last'`. Main's is the better contract — it names the row instead of relying on its position — and it is the one already on `main`, so `initialFocus` is gone and the shared `confirmDestructive` here marks its Cancel row `autofocus: true`. Every confirmation in the tree (panel delete, Dashboard delete, orphaned-variable delete) now opens on Cancel through one mechanism. **`src/ui/dashboard-tree.ts`** — main annotated and fixed `buildDeleteButton`'s inline confirm; #494 had already replaced every inline confirm with `confirmDestructive`. Kept the shared builder and carried main's fix into it. **`tests/unit/dashboard-tree.test.ts`** — main edited a test for the `⋯` trigger, which #494 removed; that test cannot survive, so it is dropped. Two things it carried were NOT dropped: main's #501 Cancel-autofocus test is ported to the orphaned-variable trash as it still exists, and the leaked `vi.useFakeTimers()` it fixed in passing is now closed as a class — a file-level `afterEach(() => vi.useRealTimers())`, since two tests in this file still leaked (one of them pre-existing, one of them mine). **`CHANGELOG.md`** — both entries kept; #494's now points at #501 for the confirm-focus mechanism rather than describing it twice. `npm test` 6070 passed on the merged tree, per-file coverage gate held. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018mujm1kW7jDGpTEfscndcU
…ger-reveal race Review round 5. Four findings, each verified against the code and sabotage-checked before landing. **Repeated activation could leave the trigger's `aria-expanded` stuck `"false"` while its replacement dialog was open.** Both pencils set `aria-expanded="true"` before calling `openMetadataDialog`, which force-closes any dialog already open — and that closing dialog's own `onClose` resets the SAME trigger back to `"false"`, landing after the "true" this same click set. Since the trigger's hover-reveal CSS keys off exactly that attribute, the result was a trigger invisible (and unfocusable) for as long as the replacement stayed open. Fixed by closing the existing shell before setting the attribute, not after. Added a real-browser e2e regression for each pencil, dispatching repeated `.click()` directly (the fair proxy for the keyboard autorepeat that is the only real way to reach a trigger a modal backdrop already covers) and sabotage-verified against the pre-fix code. **A duplicated Dashboard or tile id could still commit through the pencil while the neighbouring trash already refused it.** The tree's own availability rule and `ownedByPanel` both asked "is there at least one match" rather than "is there exactly one" — so two Dashboard documents sharing an id, or two tiles of the same Dashboard sharing an id (even referencing different queries), passed the #427 ownership check untouched: each query still looked, independently, like its sole owner. Both now count Dashboard and Dashboard-local tile ids during the same pass that counts query ids, and `ownedByPanel` resolves through `findDashboardStrict` plus an exact-one-tile filter instead of a loose `.some(...)` — the same resolution `removeDashboardPanel` already used. **A whitespace-only tile description masked the query's own description with nothing, silently.** The viewer resolved `tile.description || query description` untrimmed, while the tree's own override-warning note already trimmed before deciding whether to show a caveat — so a `" "` description was schema-legal, masked the query field exactly like a real override, and never triggered the warning that a real override would have. Trimmed the viewer's resolution to match its own title-trim precedent (#476). **A duplicated unit test.** Two consecutive, identical "focuses Cancel by default" tests — merge-resolution debris. Removed one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LVPCmA3tJHwv6a25MRkJs
…an-delete outcome Review round 6. Two findings, each verified against the code and sabotage-checked before landing. **A duplicated Dashboard or tile id still broke keyboard/navigation identity.** Round 5 made edit/delete agree with delete's own strict duplicate-id refusal, but the row's own `row.key` — and thus the `data-key` every focus restoration, drag highlight and `syncRovingTabindex` resolve by — still collapsed two ambiguous rows onto one key. `syncRovingTabindex` sets `tabindex="0"` on every node whose `data-key` matches, so a shared key put BOTH duplicate rows in the Tab order at once; every other lookup is a first-match `querySelector`, so focus restoration and drag highlighting silently picked whichever node was first in the DOM. Malformed duplicates now get distinct `:dup:<occurrence>` presentation keys — separate from the (still ambiguous) `dashboardId`/`tileId` pair their unavailable actions still name — while every child row built from a Dashboard's key (its groups, variables and panels) inherits the same disambiguation automatically. A Panel row's `double`/`shift` (View/Edit Dashboard-focus navigation, addressed by dashboard id + tile id) are nulled under the same ambiguity: the Dashboard viewer's own tile-focus lookup is keyed by tile id alone and would otherwise resolve the OTHER duplicate. Every drop target the ambiguity could reach — the Dashboard row, its Panels group, and a Variable row — is withheld too. Added a rendered-tree unit test asserting exactly one `tabindex="0"` with duplicated Dashboard and tile ids, sabotage-verified against the pre-fix key format. **Orphan-variable deletion discarded every commit outcome.** `runDestructive` fired `commitVariableConfig` and dropped its result with `void` — the one destructive control that predates round 5's `reportRemoval` pattern for the Dashboard and Panel trash. A concurrent Dashboard deletion, a Dashboard that became a duplicate id mid-flight, or a storage rejection all committed nothing and reported nothing: the confirmation just closed. It now awaits the outcome and reports it through the same toast path (`reportVariableConfigRemoval` + `variableConfigMessage`, mirroring `dashboardDeleteMessage`), and the orphan-delete action is itself withheld — via `unavailableAction`, matching the Dashboard row's own pencil/trash — when the Dashboard id is already known to be ambiguous. Added tests for both the aborted and the rejected outcome, each sabotage-verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019LVPCmA3tJHwv6a25MRkJs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Part of #494 (revised #429 phase 4), and the four post-merge review findings on #495.
The Dashboard tree's rows act directly now
Dashboard and Panel rows expose their operations as real buttons instead of hiding them
behind a
⋯menu, following the Library Query row's pencil/trash vocabulary. Noproduction row renders an overflow menu any more, so
DashboardTreeMenuItem,buildMenuButtonand the menu CSS are deleted rather than left as an unused abstraction.menu,deletableandrenamable— three expressions of one idea — collapseinto
actions: DashboardTreeAction[]: accessible name, tooltip, a fully-resolved target(stable ids only), an
unavailablereason, and the confirmation sentence for thedestructive ones. Availability is the Separate Library queries from dashboard-owned query copies #427 exactly-one-owner rule, answered once here
through
buildQueryOwnershipIndex; the view never re-derives it.OwnershipWorkspace.queriesnarrows to
{ id }so the tree's loosened projection types can be indexed by that same rule.dashboard/application/dashboard-removal.ts).removeDashboardPanelremoves a tile and exactly the dedicated query it owns — proving ownership first, composing
removeTileMembershipfor layout normalization and grid-fallback regeneration, bumping therevision exactly once.
removeDashboardDocumentremoves a document plus the queries its owntiles own, keeping any query another Dashboard also references. An orphaned
variableConfigsentry deliberately survives a panel delete (Dashboard tree: replace overflow menus with direct Panel focus, edit and delete actions (#429 phase 4) #494 non-goal).
read-latest transform, so a dialog or confirmation that went stale while it was open
commits nothing and says so. The panel pencil edits the tile's owned query through
renameSaved— inheriting its linked-tab reconciliation — with a new dequeue-timeguardon
patchSavedSpecproving the tile still owns it.buildActionButtonper model action. An unavailable control still renders,aria-disabledand inert, with the reason as its tooltip — a row's vocabulary must notshrink when its data is malformed. Destructive actions confirm through the same anchored-menu
primitive Replace curated Dashboard filters with inferred Variables and batched option queries #447 used, and the confirmation opens on Cancel. Deleting a panel moves keyboard
focus to the next sibling, else the previous, else the Panels group.
Deferred: the Open-in-Dashboard focus button. It dispatches
focus: { kind: 'tile' }, which #438 proves is a no-op on flow-layout KPI tiles, and #494forbids shipping a known no-op focus action. The capability is not lost — double-click and
Shift-click still focus a tile — only its promotion to a button waits. Owner decision taken at
the start of this phase; #494 stays open for it.
The four #495 review findings
the list and its Enter arm runs the focused row's action, so Enter on the pencil opened the
Dashboard — and could swallow the button's own activation on the way out. The
⋯and theorphan-variable trash shared the bug. Fixed in two independent layers, each separately tested.
and keeps the card open with the typed text on every unsuccessful outcome, showing one
targeted diagnostic inline; the same mutation cannot be submitted twice.
application/dashboard-create.tsis the single one; both entry points report identically andkeep their own reveal policy. The placeholder previously said nothing on a rejected commit.
role="dialog",aria-modal,aria-labelledbyadded.Review pass
Two independent read-only reviews (correctness + accessibility) ran over the branch. Everything
they surfaced is fixed in the last commit — a real focus regression (a successful commit repaints
the tree, detaching the trigger the dialog captured), a Dashboard delete that stranded focus, a
confirmation that opened on its destructive item, an unavailable delete that stopped looking like
one — plus four assertions that could not fail (the same-row
cancelFor, the glyph mapping, twonot.toBe('')checks, and the e2e post-delete focus check). Each fix is sabotage-checked.Verification
Also driven in the real served app against local ClickHouse 26.6: created a Dashboard from the
File menu (one toast, from the unified command), assigned a Library query, opened the Dashboard,
renamed the panel from the tree pencil — the rendered tile heading and the tree row both followed —
then deleted the panel from the tree while its Dashboard was on screen: the tile disappeared from the
rendered Dashboard, the row went with it, focus fell back to the Panels group, the Library source
query stayed untouched, and the console stayed clean.
Checklist
npm testpasses (the per-file coverage gate is non-negotiable)npm run buildsucceeds (single-filedist/sql.html)src/core/, network insrc/net/(injected fetch), DOM insrc/ui/CHANGELOG.md([Unreleased]) updated if behavior or the deployed surface changed